Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed 2 unnecessary stubbings in HomekitRootTest.java #187

Closed

Conversation

ARUS2023
Copy link

@ARUS2023 ARUS2023 commented Oct 6, 2023

Pull Request Checklist

Please confirm that you've done the following when opening a new pull request:

  • For fixes and other improvements, please reference the GitHub issue that your change addresses.
  • For fixes, optimizations and new features, please add an entry to the CHANGES.md file.
  • Run mvn compile before committing, so that the auto-code formatter will format your changes consistently with the rest of the project.

In our analysis of the project, we observed that

  1. 1 stubbing that stubbed getId is created in HomekitRootTest.setup but never executed in 5 tests HomekitRootTest.testWebHandlerStarts, HomekitRootTest.testWebHandlerStops, HomekitRootTest.testAdvertiserStarts, HomekitRootTest.testAdvertiserStops, HomekitRootTest.testAddIndexOneAccessory;
  2. 1 stubbing that stubbed startis created in HomekitRootTest.setup but never executed in 3 testsHomekitRootTest.verifyRegistryAdded,HomekitRootTest.verifyRegistryRemoved, HomekitRootTest.testAddIndexOneAccessory.

Unnecessary stubbings are stubbed method calls that were never realized during test execution. Mockito recommends to remove unnecessary stubbings (https://www.javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/exceptions/misusing/UnnecessaryStubbingException.html).

We propose below a solution to remove the unnecessary stubbing.

@ccutrer
Copy link
Collaborator

ccutrer commented Oct 9, 2023

Why did you split HomeKitRootTest.java into 4 files. If they're split by functionality, please name them more descriptively than "second", "third", "fourth".

@ARUS2023
Copy link
Author

Hello,

We divided the HomeKitRootTest.java into four separate files to enhance the test organization and eliminate unnecessary stubbings in the setUp method. This separation allows us to ensure that stubbing is only created when the tests actually use it.

To clarify, the reasons for splitting these files are as follows:

  1. SecondHomeKitRootTest.java: The tests in this file do not execute the stubbing that is used to stub the getId method in the setUp method.
  2. ThirdHomeKitRootTest.java: Similarly, the tests in this file do not execute both stubbings in the setUp method.
  3. FourthHomeKitRootTest.java: In this case, the tests do not execute the stubbing that stubs the start method in the setUp method.

We understand that the class names may not be as descriptive as desired. We created and used an automated tool to identify and resolve unnecessary stubbings. If you would like us to provide more meaningful names for these test classes, please let us know, and we will be happy to update them accordingly.

@ccutrer
Copy link
Collaborator

ccutrer commented Oct 10, 2023

Okay, so you're saying the stubbings are not used in some specs, not that they're not used at all. I'd much rather have a few sometimes-unnecessary stubbings than introduce all of the duplication of near-identical setups, and no clear reason why some tests are in one file and not another. If you can address those issues, I'll consider this PR.

@ARUS2023
Copy link
Author

ARUS2023 commented Oct 13, 2023

Thank you for your reply. We understand your preference. As we mentioned before, we have created and used an automated tool. This tool identifies and removes unnecessary stubbings, but importantly, it also empowers developers with a choice: they can either duplicate the test class to address the issue or retain the original structure.

We believe this solution is suitable because we group the tests into different test classes according to the settings/prerequisites required by the tests. We submitted this pull request because (in the past and in a different project) we found a case in which deleting an unnecessary stub led to a problem in the test.

@ccutrer ccutrer closed this Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants